Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use process.getActiveResourcesInfo() in process metrics #485

Merged

Conversation

RaisinTen
Copy link
Contributor

We should use process.getActiveResourcesInfo() here because it is a
public alternative of the private APIs process._getActiveHandles() and
process._getActiveRequests().

Refs: https://nodejs.org/api/process.html#processgetactiveresourcesinfo

Signed-off-by: Darshan Sen [email protected]

@RaisinTen
Copy link
Contributor Author

cc @SimenB

Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, but the tests need to be skipped in Node.js <v17 for CI to pass.

It would be great to provide upgrade guidance in the changelog since the metric name (and maybe labels?) change. (Also noting that this PR is semver major for that reason.)

lib/metrics/processResources.js Outdated Show resolved Hide resolved
We should use process.getActiveResourcesInfo() here because it is a
public alternative of the private APIs process._getActiveHandles() and
process._getActiveRequests().

Refs: https://nodejs.org/api/process.html#processgetactiveresourcesinfo

Signed-off-by: Darshan Sen <[email protected]>
@RaisinTen RaisinTen force-pushed the use-process.getActiveResourcesInfo branch from 0711bc5 to 1fe6ef6 Compare January 11, 2022 14:34
Copy link
Collaborator

@zbjornson zbjornson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you!

@zbjornson zbjornson merged commit 3b139f9 into siimon:master Jan 11, 2022
@RaisinTen RaisinTen deleted the use-process.getActiveResourcesInfo branch January 12, 2022 01:22
@zbjornson
Copy link
Collaborator

@RaisinTen - @cjihrig suggested that we actually keep both of these sets of metrics for now: https://twitter.com/cjihrig/status/1481433837255155717. What do you think?

@RaisinTen
Copy link
Contributor Author

@zbjornson yes, I agree that we should keep both the set of metrics for now. I sent #488 to do so. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants